Add test for ProtectedResourceMetadataParsing #1236
Conversation
There was a problem hiding this comment.
This PR is not testing the fact that the new fields are supported. It's testing that the class is properly validated, which it seems we are testing Pydantic here. The same can be said about the tests above.
I think an ideal test would be to use the new added literal fields in an auth flow. But that's an issue that comes from before, because we actually don't have tests for it...
Anyway, I'm okay with either merging or closing this.
Co-authored-by: Paul Carleton <paulcarletonjr@gmail.com>
Co-authored-by: Paul Carleton <paulcarletonjr@gmail.com>
Kludex
dismissed
their stale review
Actually, I don't think this is the way to go.
…resource checking
|
@Kludex tests updated |
|
@Kludex done with the requested changes |
|
Maybe we could benefit more if we move the test you added to a new file, so we can actually don't mix bad practices with good ones. |
|
I will create a separated file, actually could be directly in the test/server/auth/test_proctected_resource.py |
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
|
As per your advice, I removed the test that is just on the model, and fixed naming. |
I don't think you get anything from it, it's more often seen in old test suites and when using I prefer to stick with what modern packages do: not have wrapper classes. This is my opinion tho. |
Motivation and Context
Add tests cases for insurring ProtectedResourceMetadata validation is correctly done
How Has This Been Tested?
with pytest
Types of changes
Checklist